Skip to content

Fix __add__: mutated arguments after operation - #178

Merged
weiwei merged 3 commits into
weiwei:masterfrom
miguelflor:master
Jun 5, 2026
Merged

Fix __add__: mutated arguments after operation#178
weiwei merged 3 commits into
weiwei:masterfrom
miguelflor:master

Conversation

@miguelflor

@miguelflor miguelflor commented May 12, 2026

Copy link
Copy Markdown
Contributor

This started with a issue #177 that I posted.
So I recommend reading it to understand the description of this PR.

Description

I found that after making the add on the Object JUnitXml the arguments would mutate, making the second add with the same arguments different. Which I believe is not desirable.

Problem 1

The behavior described in the issue happens because of the function .append(child) of etree.
The etree can come from two libraries as you can see form the snippet taken from the source code:

try:
    from lxml import etree
except ImportError:
    from xml.etree import ElementTree as etree

When a the function .append(child) is called and the child already as a parent, both libraries operate in different ways.

lxml

With this library when the .append(child) is trigger it moves each child from the original tree to the new tree.
That explains why in the code I shared in the PR, after the first + in the logger all the testsuites where moved to a new object and latter garage collected. When the +is called again both vars don't have any tests.

xml.etree

This is trickier.
the behavior is actually simpler just gives the child to the new parent, but the child will have two parents at the same time. Although I could not find any error I believe that this can be prone to error.

Problem 2

This problem arises when the arguments of the __add__ where considered under the function __eq__ that they had equal suites.
Because of the call _add_testcase_no_update_stats(case), it changes the suites making it with duplicate cases.
This error only happens when using xml.etree, and that's because the values where not move at first.

def add_testsuite(self, suite):                                   
    for existing_suite in self: 
        if existing_suite == suite: # Considered equal
            for case in suite:
                existing_suite._add_testcase_no_update_stats(case) # Adds to the suite permanently
            return

Test

The test made covers both problems at the same time (:

Fix

The fix is actually pretty simple, I just called deepcopy(suite) in the beginning of the function add_testsuite(self, suite).

Additional Notes

In the end a lot of yapping for a simple solution. If you have any questions feel free to ask.

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Test Results

  8 files  ±0    8 suites  ±0   4s ⏱️ ±0s
129 tests +1  128 ✅ +1   1 💤 ±0  0 ❌ ±0 
735 runs  +5  716 ✅ +5  19 💤 ±0  0 ❌ ±0 

Results for commit 88cc9f9. ± Comparison against base commit ab3451b.

@weiwei weiwei left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @miguelflor !

@weiwei
weiwei merged commit c907926 into weiwei:master Jun 5, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants